feat(model): add the GitHub Copilot CLI as a backend - #202
Conversation
…backends configs/_base_/default.yaml ships optimizer_backend: openai_chat and arget_backend: openai_chat. Both entry points only resolved a high-level --backend label when a role was missing, so for any run using the shipped defaults the label was silently discarded and the run executed on openai_chat. skillopt-train --config configs/searchqa/default.yaml --backend cursor ... [model config] backend=cursor_exec optimizer=... (openai_chat) target=... (openai_chat) train.py guarded on "is either role unset?"; eval_only.py used cfg.setdefault(), which is equally a no-op once the key exists. A role left at the default openai_chat now counts as unset so the label wins, while a role the operator explicitly pointed elsewhere still takes precedence. The trainer's resolution moves to a module-level _resolve_role_backends() so it is testable -- it previously sat inline inside Trainer.train().
Adds two backends. `copilot_chat` drives the Copilot CLI as a chat model and can fill either role, so `--backend copilot` selects it for BOTH optimizer and target -- the CLI carries its own sign-in, which makes that the only fully local configuration: a complete train/eval loop with no cloud API key. `copilot_exec` is the separate target-only execution harness, alongside the existing codex/claude/cursor harnesses. Verified end to end on SearchQA with no credentials configured: baseline eval, rollout, reflect, aggregate, select, update and gate all execute against the local CLI. Safety: chat calls disable built-in MCP servers and custom instructions so the model sees only the prompt SkillOpt sends, and never pass --allow-all-tools. Unlike the other exec harnesses, `copilot_exec` does NOT grant unattended tool use by default -- it requires an explicit `copilot_exec_allow_all_tools` opt-in, because a file-edit rollout is the only case that needs it. Two caveats worth knowing before use: the CLI is an agent rather than a completions endpoint, so expect roughly 20-40 s per call; and it reports no token counts, so usage totals are zero for these backends. Depends on the --backend resolution fix: without it, --backend copilot is discarded whenever the base config sets both role backends.
There was a problem hiding this comment.
Pull request overview
This PR adds support for running SkillOpt against the local GitHub Copilot CLI in two modes: copilot_chat (chat backend usable as optimizer and target for a fully local run) and copilot_exec (target-only exec harness). It also updates CLI/config wiring, documentation, and tests to cover the new backends and to ensure --backend correctly overrides role backends pinned by the base config.
Changes:
- Introduce
copilot_chatbackend that drives thecopilotCLI as a chat model (optimizer/target), with MCP servers and custom instructions disabled. - Add
copilot_execexecution harness (target-only) with explicit opt-in gating for unattended tool use (--allow-all-tools). - Extend config/CLI/docs/tests and adjust role-backend resolution so
--backendis not silently ignored when base defaults pin roles.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_role_backend_resolution.py | New regression tests for role-backend resolution behavior. |
| tests/test_copilot_exec_backend.py | Adds unit tests for Copilot backend normalization, configuration wiring, and exec/chat safety flags. |
| skillopt/model/copilot_backend.py | Implements the Copilot CLI chat backend and JSONL parsing. |
| skillopt/model/common.py | Registers Copilot backends in backend aliases/default-model map. |
| skillopt/model/codex_harness.py | Adds run_copilot_exec harness + dispatch in run_target_exec. |
| skillopt/model/backend_config.py | Adds Copilot-related env/config plumbing and backend whitelists. |
| skillopt/model/init.py | Wires Copilot backends into set_backend / chat routing. |
| skillopt/engine/trainer.py | Adds _resolve_role_backends and uses it during eval env construction. |
| skillopt/config.py | Adds config-flattening keys for Copilot settings. |
| scripts/train.py | Exposes Copilot backends and flags via legacy CLI args. |
| scripts/eval_only.py | Fixes --backend overriding behavior and adds Copilot CLI/config wiring. |
| README.md | Updates backend list to include Copilot backends. |
| docs/reference/config.md | Documents Copilot backend availability and config keys. |
| docs/reference/api.md | Documents Copilot backends in the public API reference tables. |
| docs/guide/configuration.md | Adds Copilot CLI backend explanation, env vars, and safety notes. |
| configs/base/default.yaml | Adds Copilot config fields and comments to base defaults. |
| CHANGELOG.md | Notes new Copilot backends and their behavior/safety properties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lufen
changed the title
…chat The claude/claude_chat branch used 'x = x or default', but the base config pins both roles to the truthy 'openai_chat', so --backend claude was still silently ignored -- the very bug this resolver fixes for the other backends. Switch it to the _ROLE_BACKEND_DEFAULTS check used elsewhere, and extend the base-config regression parametrization to cover claude and claude_chat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
skillopt/model/backend_config.py:115
- This return statement is now long enough to be hard to read and is inconsistent with the multi-line style used for other backend sets in this module. Consider formatting it as a multi-line set literal to keep it readable and reduce churn when adding/removing backends.
def is_target_chat_backend() -> bool:
return TARGET_BACKEND in {"openai_chat", "claude_chat", "qwen_chat", "minimax_chat", "openai_compatible", "copilot_chat"}
skillopt/model/init.py:114
- get_backend_name() has a special-case for copilot_exec but not for the fully-local copilot_chat backend. When both roles are copilot_chat, this currently falls through to the generic "optimizer+target" string ("copilot_chat+copilot_chat"), which is inconsistent with the other unified backends and can confuse logs/telemetry that expect a canonical backend label.
if optimizer == "openai_chat" and target == "copilot_exec":
return "copilot_exec"
if optimizer == "openai_compatible" and target == "openai_compatible":
return "openai_compatible"
return f"{optimizer}+{target}"
skillopt/model/backend_config.py:84
- This target-backend whitelist is now a very long single line, unlike the optimizer whitelist above, and is likely to violate line-length/style checks. Wrapping it like the optimizer whitelist keeps formatting consistent and easier to edit when adding more backends.
This issue also appears on line 114 of the same file.
TARGET_BACKEND = normalize_backend_name(backend or "openai_chat")
if TARGET_BACKEND not in {"openai_chat", "claude_chat", "qwen_chat", "minimax_chat", "openai_compatible", "copilot_chat", "codex_exec", "claude_code_exec", "cursor_exec", "copilot_exec"}:
Addresses re-review: get_backend_name() special-cased copilot_exec and the other unified chat backends (claude_chat, qwen_chat) but not copilot_chat, so a fully-local run reported the generic 'copilot_chat+copilot_chat'. Return the canonical 'copilot_chat' for both-role copilot, and assert it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skillopt/model/backend_config.py:35
COPILOT_EXEC_ALLOW_ALL_TOOLSis read directly from the environment, butget_copilot_exec_config()only accepts values '0' or '1'. If a user sets the env var to a common boolean string like 'true'/'false', SkillOpt will raiseValueErrorat runtime. Consider normalizing the env var on load the same way other boolean-ish flags are parsed so 'true' becomes '1' and 'false' becomes '0'.
COPILOT_EXEC_ALLOW_ALL_TOOLS = os.environ.get("COPILOT_EXEC_ALLOW_ALL_TOOLS", "0")
Re-review catch: the module-level read took the env var raw, so setting COPILOT_EXEC_ALLOW_ALL_TOOLS=true/false (without calling configure_copilot_exec) made get_copilot_exec_config() raise ValueError. Normalize it through the existing _parse_bool helper to '0'/'1' (unknown values fall back to the safe '0'), matching the other boolean-ish exec flags. Regression added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Second re-review follow-up in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skillopt/model/codex_harness.py:1407
- When the Copilot CLI emits no assistant messages (empty/invalid JSONL), the loop records each attempt’s stdout/stderr in
all_raw, but the finalRuntimeError(last_error)drops that context. Unlike other exec harnesses (e.g., cursor/codex) this makes a “no response” failure hard to debug because callers get neither persisted artifacts nor any CLI output.
combined = "\n\n".join(all_raw)
raise RuntimeError(last_error)
Re-review catch: the final failure path computed 'combined' from all_raw and then discarded it, raising a bare 'Copilot CLI returned no response'. Unlike the cursor/codex harnesses, copilot_exec persists no artifacts, so an empty or invalid JSONL stream left the caller with nothing to debug. Append a bounded (4000-char) tail of the captured stdout/stderr to the error. Regression added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Third re-review follow-up in Suites green (509 passed). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
skillopt/model/common.py:28
default_model_for_backend()returns an empty string forcopilot_chat/copilot_exec. This propagates into the CLI entrypoints (e.g. scripts/eval_only.py usesdefault_model_for_backend(backend)as a fallback for optimizer/target deployments), and can leave deployments empty (notably for--backend copilot_exec, where the optimizer is stillopenai_chat). Returning a real default here (or omitting these keys to fall back to the Azure/OpenAI default) avoids accidentally configuring an empty deployment.
"copilot_exec": "",
"copilot_chat": "",
skillopt/model/codex_harness.py:1384
- On
subprocess.TimeoutExpired, this captures onlyexc.stdoutand dropsexc.stderr. The other exec harnesses include stderr in their raw capture, and it’s important for debugging when Copilot emits errors to stderr before timing out.
except subprocess.TimeoutExpired as exc:
raw = exc.stdout or ""
if isinstance(raw, bytes):
raw = raw.decode("utf-8", "replace")
all_raw.append(f"===== COPILOT CLI ATTEMPT {attempt + 1} =====\n{raw}")
raise
…out stderr
Two re-review catches:
- _BACKEND_DEFAULT_MODELS mapped copilot_chat/copilot_exec to the empty string.
That table also feeds the shared Azure deployment fallback in the entry
points -- cfg.get("optimizer_model", default_model_for_backend(backend)) --
and the shipped base config sets no optimizer_model/target_model, so the
fallback is reached: --backend copilot_exec configured an EMPTY optimizer
deployment even though that role is still a real openai_chat model. Drop the
entries so they fall back to the Azure default; the CLI's own model continues
to come from copilot_chat_optimizer_model / copilot_chat_target_model.
- run_copilot_exec dropped exc.stderr on TimeoutExpired; the codex/cursor
harnesses all capture it, and Copilot can report the cause there before
timing out. Capture it the same way.
Regressions added for both.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fourth re-review follow-up in Empty optimizer deployment. Timeout stderr. Regressions added for both; suites green (510 passed). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skillopt/model/codex_harness.py:1391
- In
run_copilot_exec, theTimeoutExpiredhandler spends effort normalizing/combining stdout+stderr and appending toall_raw, but then immediately re-raises. Sinceall_rawis never returned/persisted on this path, the combinedrawis discarded and the comment about “captur(ing) it” is misleading. Either persist/propagate the combined output, or (simpler) remove the dead code and just re-raise like the other exec harnesses.
except subprocess.TimeoutExpired as exc:
raw = exc.stdout or ""
if isinstance(raw, bytes):
raw = raw.decode("utf-8", "replace")
# Copilot can report the cause on stderr before timing out; the
# other exec harnesses capture it, so this must too.
err = exc.stderr or ""
Re-review catch: the TimeoutExpired handler normalized stdout/stderr into all_raw and then re-raised, but all_raw is local and never returned or persisted on that path -- unlike the codex/cursor harnesses, which persist artifacts before raising. The work was dead and the comment misleading. TimeoutExpired already carries .stdout/.stderr to the caller, so simply re-raise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fifth re-review follow-up in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
configs/base/default.yaml:33
configs/_base_/default.yamlpinscopilot_exec_allow_all_tools: false, and bothtrainer.pyandscripts/eval_only.pyunconditionally pass that value intoconfigure_copilot_exec(...). This overwritesCOPILOT_EXEC_ALLOW_ALL_TOOLSinos.environon every run, so the documented env-var opt-in (COPILOT_EXEC_ALLOW_ALL_TOOLS=1) cannot take effect unless the config also flips the YAML key. Usingnullhere would preserve the safe default (env default is still false) while allowing an explicit env opt-in or per-run config override.
copilot_exec_allow_all_tools: false # copilot_exec only; required for file-edit rollouts
…opt-in Re-review catch: configs/_base_/default.yaml pinned copilot_exec_allow_all_tools: false, and trainer.py / eval_only.py pass that value straight into configure_copilot_exec(). A non-None value overwrites COPILOT_EXEC_ALLOW_ALL_TOOLS in os.environ on every run, so the documented env-var opt-in could never take effect. Use null instead, matching the blank-uses-env convention of the neighbouring keys. The safe default is unchanged (the env default is off), an explicit --copilot_exec_allow_all_tools still wins, and the env opt-in now works. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Sixth re-review follow-up in
Changed to
Regression added asserting the base config leaves it |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skillopt/model/init.py:256
- The NotImplementedError message is now outdated:
copilot_chatis a supported chat backend, but it isn’t listed in thechat_targetsupported-backends message. This can confuse users when they hit this error with an unsupported backend.
"chat_target is only supported with target_backend=openai_chat, claude_chat, qwen_chat, minimax_chat, "
Re-review catch: copilot_chat is a supported chat target, but the NotImplementedError raised by chat_target (and the matching one in chat_target_messages) still omitted it from the supported-backends list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
skillopt/model/copilot_backend.py:216
- This backend records into a TokenTracker but does not expose the standard get_token_summary()/reset_token_tracker() helpers that the rest of the backends provide. That makes copilot_chat usage hard to report/reset consistently and blocks wiring it into skillopt.model.get_token_summary().
def chat_target_messages(
messages: list[dict[str, Any]],
max_completion_tokens: int = 16384,
retries: int = 5,
stage: str = "target",
reasoning_effort: str | None = None,
timeout: float | None = None,
**_ignored: Any,
) -> tuple[str, dict[str, int]]:
del max_completion_tokens, reasoning_effort
config = get_copilot_chat_config()
return _chat_impl(
_messages_to_prompt(messages),
retries,
stage,
model=str(config.get("target_model") or ""),
timeout=timeout,
)
skillopt/model/init.py:13
- copilot_chat is now a supported backend (and copilot_backend records TokenTracker data), but skillopt.model.get_token_summary() / reset_token_tracker() still aggregate only OpenAI/Claude/Qwen/MiniMax/OpenAI-compatible/Codex. As a result, runs using copilot_chat will omit call counts from the per-step token snapshots in trainer.py and from the overall token summary.
from skillopt.model import azure_openai as _openai
from skillopt.model import claude_backend as _claude
from skillopt.model import codex_backend as _codex
from skillopt.model import copilot_backend as _copilot
from skillopt.model import minimax_backend as _minimax
from skillopt.model import openai_compatible_backend as _openai_compat
from skillopt.model import qwen_backend as _qwen
Re-review catch: copilot_backend recorded into a TokenTracker but exposed neither get_token_summary() nor reset_token_tracker(), so model-level aggregation skipped it and copilot_chat runs omitted call counts from the per-step token snapshots and the run summary. Add both helpers and wire them into model.get_token_summary() / reset_token_tracker(). The CLI still reports no token counts -- that caveat is unchanged -- but the call counts are real and now surface. Also reset the tracker in the copilot test fixture: those tests record calls, and now that the tracker feeds the aggregate, leftover state leaked into test_combined_token_summary_counts_each_backend_once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up in
To be clear about the caveat in the PR description: the CLI still reports no token counts, so the token fields remain zero — but the call counts are real and now surface like every other backend. One thing worth flagging: wiring this in surfaced a latent test-pollution bug. The copilot tests record calls, and once the tracker fed the aggregate, that leftover state leaked into Suites green (512 passed), and verified order-independent. |
|
Thanks for the careful follow-ups and for the end-to-end SearchQA validation. The Copilot CLI direction is valuable, but the current head still has a few cross-backend contract and safety blockers, so I think we should hold the merge for now:
Two documentation details should also be corrected while updating this:
Once these are addressed, we will be happy to re-review. Thank you again for the thorough iteration on this integration. |
Adds two backends.
copilot_chatdrives the Copilot CLI as a chat model andcan fill either role, so
--backend copilotselects it for BOTH optimizer andtarget — the CLI carries its own sign-in, which makes that the only fully local
configuration: a complete train/eval loop with no cloud API key.
copilot_execis the separate target-only execution harness, alongside theexisting codex/claude/cursor harnesses.
Verified end to end on SearchQA with no credentials configured: baseline eval,
rollout, reflect, aggregate, select, update and gate all execute against the
local CLI.
Safety: chat calls disable built-in MCP servers and custom instructions so the
model sees only the prompt SkillOpt sends, and never pass --allow-all-tools.
Unlike the other exec harnesses,
copilot_execdoes NOT grant unattended tooluse by default — it requires an explicit
copilot_exec_allow_all_toolsopt-in, because a file-edit rollout is the only case that needs it.
Two caveats worth knowing before use: the CLI is an agent rather than a
completions endpoint, so expect roughly 20–40 s per call; and it reports no
token counts, so usage totals are zero for these backends.